-
Notifications
You must be signed in to change notification settings - Fork 105
[HUD] Display on hud table the autorevert decisions #7304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
jeanschmidt
commented
Oct 6, 2025
- Highlight hud autorevert decisions on main hud table with a different background
- Add a banner on hover for autoreverted commits on main hud table
- Special background for signals that are flagged to autorevert, making them easier to identify
- Adds a banner on commit page about the autorevert and the identified signals
Signed-off-by: Jean Schmidt <[email protected]>
…nschmidt/hud_revert_info
Signed-off-by: Jean Schmidt <[email protected]>
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Signed-off-by: Jean Schmidt <[email protected]>
FYI: test error is unrelated to the change, it is a library that changed behaviour and I am not managing this in this PR |
// assert log | ||
expect(logSpy).toHaveBeenCalledWith( | ||
`Warning: Error parsing JSON:SyntaxError: Expected property name or '}' in JSON at position 1 for data string '{{}dsad}'` | ||
`Warning: Error parsing JSON:SyntaxError: Expected property name or '}' in JSON at position 1 (line 1 column 2) for data string '{{}dsad}'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accidental change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no tried to fix, but the message is not correct, I can remove the change
); | ||
} else { | ||
const job = rowData.nameToJobs.get(name); | ||
const jobFullName = job?.name || name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally this logic should be extracted into a separate method
Note it wasn't a accidental changes, just gave up on fixing the test for the newer version of the library. End up updating anyway.
Yeah, ok, good point, extracted the logic and reused for both the grouped and expanded views, I wasn't going to support the expanded views, but I guess this is a good point that support it.
changed to orange |
}} | ||
title={ | ||
isAutorevert | ||
? "This job triggered an autorevert" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
> | ||
{job.name} | ||
{isAutorevert && " ⚠️ (triggered autorevert)"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand where this is shown
href={job.htmlUrl} | ||
target="_blank" | ||
rel="noreferrer" | ||
style={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ideally this should be css classes instead of inline styles
style={{ | ||
display: "block", | ||
fontWeight: isAutorevert ? "bold" : "normal", | ||
color: isAutorevert ? "#d73a49" : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think it makes sense to make it a css class, because currently the style is duplicating in two places
} | ||
|
||
// Handle case where arrays might be undefined or have different structure | ||
const workflows = autorevertData.workflows || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really necessary? shouldn't backend have a predictable response structure?
// Handle case where arrays might be undefined or have different structure | ||
const workflows = autorevertData.workflows || []; | ||
const sourceSignalKeys = autorevertData.source_signal_keys || []; | ||
const jobIds = autorevertData.job_ids || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those are unused!